- 
                Notifications
    You must be signed in to change notification settings 
- Fork 2.3k
Explicit index resolution API #18523
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Explicit index resolution API #18523
Conversation
| ❌ Gradle check result for 09a308b: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? | 
| Codecov Report❌ Patch coverage is  Additional details and impacted files@@             Coverage Diff              @@
##               main   #18523      +/-   ##
============================================
- Coverage     73.16%   73.11%   -0.05%     
- Complexity    70842    70898      +56     
============================================
  Files          5732     5735       +3     
  Lines        324191   324586     +395     
  Branches      46922    46959      +37     
============================================
+ Hits         237186   237322     +136     
- Misses        67841    68148     +307     
+ Partials      19164    19116      -48     ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
 | 
        
          
                server/src/main/java/org/opensearch/action/support/ActionRequestMetadata.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                server/src/main/java/org/opensearch/action/support/TransportIndicesResolvingAction.java
          
            Show resolved
            Hide resolved
        
      | @saratvemulapalli @sohami @owaiskazi19 @dbwiddis @jainankitk wdyt of the proposed changes in this PR? The main goal is to offload concrete index resolution to the core instead of security knowing how to extract from all different types of requests as it does in https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/resolver/IndexResolverReplacer.java#L661-L837 | 
        
          
                server/src/main/java/org/opensearch/action/support/TransportIndicesResolvingAction.java
          
            Show resolved
            Hide resolved
        
      | @nibix  Can  | 
| 
 The current version has separate attributes for remote indices and local indices, aliases and data streams. Currently, local indices, alises and datastreams are however in one bucket. 
 Yes, wildcards will be resolved in the ResolvedIndices object (that's one of the main objectives of the class :) | 
| This PR is stalled because it has been open for 30 days with no activity. | 
| 
 Additionally, I need an API extension to let IndexNameExpressionResolver to optionally not fail on non-existant indices. This is also mostly done, I will try to push this ASAP. Another thing I still have to think about: How to test this. | 
| 
 I would be interested to learn about the details of this case, just to make sure that we are aligned | 
| 
 @kumargu to comment further, but my understanding is they need to rolve the concrete indices from the request to then determine the correct set of keys for decryption (and whether the user has access). | 
| 
 Hi @nibix , for index-level encryption we need to know - 
 | 
| FYI @nibix I found another area of the core trying to do extraction of indices from an ActionRequest in an ActionFilter. See: https://github.com/opensearch-project/OpenSearch/blob/main/plugins/workload-management/src/main/java/org/opensearch/plugin/wlm/rule/attribute_extractor/IndicesExtractor.java @kaushalmahi12 @ruai0511 FYI that IndicesExtractor is missing a lot of cases for resolving to concrete indices from a generic ActionRequest. See the IndexResolverReplacer from the security plugin which currently has the most exhaustive logic. The change @nibix is introducing in this PR is for resolving to concrete indices from any type of transport action associated with indices so WLM would benefit from these changes as well. @kkhatua tagging you as well. | 
| Hm, so is that only about  | 
| 
 Yes. Index Level Encryption can be only updated on family of  | 
e721867    to
    4266dbc      
    Compare
  
    | ❌ Gradle check result for 4266dbc: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? | 
| ❌ Gradle check result for ea276fb: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? | 
| ❌ Gradle check result for 004f937: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? | 
9e8cffa    to
    72fe3f3      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nibix Thank you for the thorough coverage of existing native transport actions in the core. With the fallback logic in the security plugin I think this paves the way towards a more official index resolution process for core actions instead of having the security plugin having to have convoluted logic to resolve to indices from all different types of ActionRequests which are not currently organized in any sensible way.
Signed-off-by: Nils Bandener <[email protected]>
Signed-off-by: Nils Bandener <[email protected]>
        
          
                server/src/main/java/org/opensearch/action/support/ActionFilter.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                server/src/main/java/org/opensearch/action/support/ActionFilter.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      Signed-off-by: Nils Bandener <[email protected]>
| ❌ Gradle check result for 1f681db: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? | 
| ❌ Gradle check result for 1f681db: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? | 
| ❌ Gradle check result for 1f681db: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? | 
The PR opensearch-project/OpenSearch#18523 needs to add a new parameter to the apply() method of the ActionFilter interface. This adds the parameter to the ActionFilter implementations. Signed-off-by: Nils Bandener <[email protected]>
The PR opensearch-project/OpenSearch#18523 needs to add a new parameter to the apply() method of the ActionFilter interface. This adds the parameter to the ActionFilter implementations. Signed-off-by: Nils Bandener <[email protected]>
The PR opensearch-project/OpenSearch#18523 needs to add a new parameter to the apply() method of the ActionFilter interface. This adds the parameter to the ActionFilter implementations. Signed-off-by: Nils Bandener <[email protected]>
The PR opensearch-project/OpenSearch#18523 needs to add a new parameter to the apply() method of the ActionFilter interface. This adds the parameter to the ActionFilter implementations. Signed-off-by: Nils Bandener <[email protected]>
Description
This PR introduces a mechanism that allows
ActionFilterimplementations to retrieve information about the actual indices a transport action will operate on - based on the current request. So far, theActionFilterhad access to the index information that is directly available in theActionRequestobjects. However, transport actions have quite varying opinions on how to interpret this information. Some evaluate index patterns, some only date math, some allow remote indices, and some only allow a single concrete index. The new mechanism creates interfaces that give transport actions the ability to communicate their interpretation explicitly.This PR does only add the interface, but not any implementation that uses the new information. The first implementation that consumes the information will be in the security plugin; a draft PR that shows how the information will be processed is located at opensearch-project/security#5399.
Intended use
The primary intended client will be the security plugin. In order to implement index-based access controls, the security plugin needs a reliable way to retrieve information about the indices a request refers to.
At the moment, this needs to be hard-coded for each known action request in the security plugin. This results in a very large class containing many special cases: https://github.com/opensearch-project/security/blob/c51ce46dc0d64326fe9f719e83f2cbb53147117a/src/main/java/org/opensearch/security/resolver/IndexResolverReplacer.java#L652
This brings several issues:
See opensearch-project/security#5367 for more details.
Reading this PR
This is is a large PR, as it adds both the interfaces and infrastructure and also the individual method implementations to
TransportActionclasses. When reviewing, you should probably start with the inferfaces and infrastructure; if you got an overview of this, you can go ahead to the individual implementations.Main components
TransportIndicesResolvingAction
This PR introduces a new Java interface that can be implemented by
TransportActions:OpenSearch/server/src/main/java/org/opensearch/action/support/TransportIndicesResolvingAction.java
Lines 14 to 30 in 09a308b
The method
resolveIndicesexpects a request object and is supposed to return the indices, aliases and/or data streams, the respective transport action would operate on, if theexecute()method is called.ActionRequestMetadata
A new class
ActionRequestMetadatais introduced which is passed as additional parameter to allActionFilter.apply()implementations:OpenSearch/server/src/main/java/org/opensearch/action/support/ActionFilter.java
Lines 56 to 63 in b008fd2
OpenSearch/server/src/main/java/org/opensearch/action/support/ActionRequestMetadata.java
Lines 21 to 76 in b008fd2
OptionallyResolvedIndices, ResolvedIndices
The
ActionRequestMetadataclass givesActionFilterimplementations access to instances ofResolvedIndicesandOptionallyResolvedIndices. The reason why the classOptionallyResolvedIndicesexists is the following: There can be situations when the resolved indices are not known. For example, that's the case when a transport action does not implement theTransportIndicesResolvingActioninterface. The existance of theOptionallyResolvedIndicesclass provides a type safe and null safe way of checking whether the information is known or not. Implementations that use the classes should implement this pattern:Still, also the
OptionallyResolvedIndicesclass implements a couple of methods which allow certain checks to be done without theinstanceofchecks.OpenSearch/server/src/main/java/org/opensearch/cluster/metadata/OptionallyResolvedIndices.java
Lines 18 to 117 in b008fd2
OpenSearch/server/src/main/java/org/opensearch/cluster/metadata/ResolvedIndices.java
Lines 31 to 573 in b008fd2
IndexNameExpressionResolver
The class
IndexNameExpressionResolverhas been extended by methods that returnResolvedIndices.Local.Concreteobjects. This allows transport actions to use the resolution logic fromIndexNameExpressionResolverboth for its internal purposes and also for implementing theTransportIndicesResolvingActioninterface.There is one fundamental different in the use of the resolution logic between the actual action execution and the implementation of
TransportIndicesResolvingAction: When the actual action is executed,IndexNameExpressionResolverperforms certain validation steps: Do indices exist, does the request index fit other specified parameters, etc. For the metadata reporting, however, we want the metadata even when the validation fails. Thus, the returnedResolvedIndices.Local.Concreteprovide methods to access validated indices and method to access unvalidated names.Related Issues
Partially resolves opensearch-project/security#5367
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.